@alecpl Same question for this: would reviews by other developers help you?

I'm not sure it would help me, but it would likely help the pull request move forward.

Are there other things we can do to ease your load? E.g. get more ticket work off your back? Or release/version/patch-management? I notice you're backporting many changes that go into the master-branch to one or two other branches, I guess that's a lot to juggle, too.

@johndoh, @mvorisek Do you feel competent to review this PR? It would help us a lot, even if you're not perfectly sure about all parts, just tell us about your opinion.

I'm not convinced that providing such configuration options is a good idea.

I know nothing about CSP headers. From a Roundcube code style point of view only I would say that

if (!in_array($csp, ['', false, 'false'])) {

is wrong and it should be

if ($csp = $this->app->config->get('content_security_policy', "default-src 'self' data: blob:; script-src 'self' 'unsafe-inline' 'unsafe-eval'; style-src 'self' 'unsafe-inline';")) {

this means, i think, in common with other config values null reverts to a hard coded default and false disables (e.g imap_ns_*).

I would also say the comment in defaults.inc.php needs to mention that content_security_policy must end in a ; or the header will be invalid when concatenated with content_security_policy_add_allow_remote

I'm not convinced that providing such configuration options is a good idea.

@alecpl Why? I thought some people might need to change the CSP (e.g. for plugins using external resources), and this way they wouldn't need to patch the code.

I know nothing about CSP headers.

@johndoh But your comments are helpful, thank you very much!

this means, i think, in common with other config values null reverts to a hard coded default and false disables

For some values that's right, but defaults.inc.php has a lot of actual default values, too. E.g. most booleans, but also oauth_user_create_map, or blankpage_url.
I would like to avoid repeating the default value, because it's pretty complex in this case – but at the same time it needs to be visible outside of the actual code as a reference. Thus I find the other way more appropriate.
Is there a declared will or style guide towards one of the default value methods?

content_security_policy must end in a ;

Thank you for that catch! I'l solve that in code so admins can't break the CSP so easily by mistake.

I would like to avoid repeating the default value,

I agree with this.

I think in the past there has been issues with defaults.inc.php not being properly updated in 3rd party packaged versions of Roundcube (which I acknowledge are not our problem) and people affected by that create issues here. The comment could suggest a value while the code has a reliable default so if defaults.inc.php was not updated there is no unpredictable behavior.

Personally I just don't like the in_array check, I think its unnecessary because it handles the edge case where someone set 'false' vs false and I think in general Roundcube does not do that so why do it for this one var.

I think in the past there has been issues with defaults.inc.php not being properly updated in 3rd party packaged versions of Roundcube

Oh, I wasn't aware of that. Thank you for that explanation! Then we maybe should have a hardcoded value for all defaults.

I changed the approach to specifying the default value in defaults.inc.php, and having a hardcoded default. And to check that these are in sync, I wrote a test. What do you think about this way?

(I'd be happy to extend this approach to more config options, it would reduce some special case handling in rcube_config, too.)

Personally I just don't like the in_array check, I think its unnecessary because it handles the edge case

I also find it ugly, but I really like solid code, which isn't vulnerable to edge cases. But maybe checking against 'false' is too much. Regarding readability I can turn the check around by using is_string($csp) && strlen($csp) > 0.